Extract IStringFmt.js from IString.js (Phase 2) - #511
Open
gouniLee wants to merge 2 commits into
Open
Conversation
Move formatting methods (format, _testChoice, _isIntlPluralAvailable, formatChoice, setLocale, getLocale) out of IString.js into a standalone IStringFmt.js module, mixed back onto IString.prototype via Object.assign. This completes Phase 2 of the split described in docs/IString-Refactoring.md (IString.js: 1158 -> 712 lines). - formatChoice's `new IString(...)` calls become `new this.constructor(...)` so IStringFmt.js never requires IString.js directly (no circular dep) - IString.prototype.constructor is restored explicitly, since replacing .prototype with an object literal drops it, which broke the this.constructor calls above - _testChoice/setLocale now call PluralUtils directly instead of reaching through the IString._fncs/loadPlurals aliases - Add js/test/root/testistringfmt.js covering format/formatChoice/ setLocale/getLocale end-to-end No functional changes intended; several pre-existing bugs in the moved code (found during review) are left untouched for a follow-up commit.
The formatting-mixin test file (added in the IStringFmt extraction commit) was never added to testSuiteFiles.js, so `ant test.root` silently skipped it entirely. Also rename it from testistringfmt.js to teststringfmt.js and its internal test names from testIStringFmt* to testStringFmt*, matching the existing teststrings.js convention of dropping the "I" prefix from IString-family class names. Also fix testStringFmtFormatCanBeAppliedTwice, which only surfaces once the file actually runs: format() returns a plain JS string, not an IString, so chaining .format() on its result needs to be re-wrapped in new IString(...) first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
ReleaseNoteshas been updated or is not needed.Node.jsandChrome Browser.Summary
Phase 2 of the IString.js split described in
docs/IString-Refactoring.md. Extracts the formatting-method cluster (format,_testChoice,_isIntlPluralAvailable,formatChoice,setLocale,getLocale) out ofIString.jsinto a newIStringFmt.js, mixed back ontoIString.prototypeviaObject.assign. No change to the public API.js/lib/IStringFmt.jsas a plain object (mirrors thePluralUtils.jspattern from Phase 1), required and mixed intoIString.prototypefromIString.jsformatChoice's internalnew IString(...)calls becomenew this.constructor(...)soIStringFmt.jsnever requiresIString.js(no circular dependency)IString.prototype.constructor = IString, which is lost when.prototypeis replaced with an object literal — required for thethis.constructorcalls above to resolve correctly_testChoice/setLocalecallPluralUtilsdirectly instead of going through theIString._fncs/IString.loadPluralsstatic aliasesjs/test/root/teststringfmt.jsand register it intestSuiteFiles.jsIString.js: 1158 → 712 lines.IStringFmt.js: 482 lines (new).Notes
This is a pure move — no behavior changes. Several pre-existing logic bugs in the moved
formatChoice/formatcode were found during review (not caused by this extraction, just carried over verbatim) and are being fixed separately in thefix-istringfmt-logic-bugsbranch, to keep the structural move and the behavioral fixes easy to review independently.This PR's base branch is
refactor-istring-plural-utils, which is #509. (Phase 1 — extractPluralUtils.js) needs to be merged first; this PR should be rebased ontodevelopment(or its base updated) once that lands.Test plan
ant test.root— 7092 assertions, all passingant test.address/test.date/test.units/test.name— consumer suites (formatChoice/format/setLocalecallers:DateFmt,DurationFmt,UnitFmt,ResBundle,NameFmt,AddressFmt) all passing, no regressionsdocs/IString-Refactoring.mdupdated to mark Phase 2 done